Fix a bug in AbsCpatureTimeExtension #248
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #248 +/- ##
==========================================
+ Coverage 87.41% 87.50% +0.09%
==========================================
Files 21 21
Lines 2113 2129 +16
==========================================
+ Hits 1847 1863 +16
Misses 230 230
Partials 36 36
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| if offset < 0 { | ||
| offset = -offset | ||
| negative = true | ||
| } | ||
| duration := time.Duration(offset/(1<<32))*time.Second + time.Duration((offset&0xFFFFFFFF)*1e9/(1<<32))*time.Nanosecond |
There was a problem hiding this comment.
Would something like:
duration := time.Duration(offset/(1<<32))*time.Second + time.Duration((offset%(1<<32)))*1e9/(1<<32))*time.Nanosecond
work instead to be branchless?
There was a problem hiding this comment.
Thanks @kevmo314 for your comment.
To accurately handle negative durations in the NTP offset conversion, the 64-bit value must undergo a 2's complement operation (^(value) + 1), the example you showed wouldn't work. I think it is challenging to implement without conditional branching imo. (FYI: We found, based on a benchmark, the unary operation (val = -val) is faster (Go does it well apparently) than ^(val) + 1. )
If you had a better solution that works, please let me know!
There was a problem hiding this comment.
Sounds good thanks for trying!
| lsb := (ns / 1e9) & 0xFFFFFFFF | ||
| msb := (((ns % 1e9) * (1 << 32)) / 1e9) & 0xFFFFFFFF |
There was a problem hiding this comment.
Same here, could we replace & 0xFFFFFFFF with % (1 << 32) to fix the issue?
Description
Reference issue
Fixes #247